Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change from chrono implementation to impl based on time crate #506

Closed

Conversation

noahcoetsee
Copy link
Contributor

@noahcoetsee noahcoetsee commented Nov 7, 2021

Change from chrono implementation to impl based on time crate

Status

  • Ready
  • Development
  • Hold

Description

Change from chrono implementation to impl based on time crate, as it has addressed the issues present in RUSTSEC-2020-0159 in its own security advisory: RUSTSEC-2020-071. This is due to chrono no longer being actively maintained.

The new implementation still logs in the exact same time format as the previous chrono implementation.

This is necessary to prevent some cargo audit issues in the CI/CL Action cycle. There are other issues (involving the same RUSTSEC advisory) currently present due to this issue in crates used by feather such as rust-simple_logger, and I have made a PR there as well to address the issue. Once that PR is (hopefully) merged, we can update the version in feather-util and minecraft-proxy. The issue is also present in simple_asn1 and I'm currently working on a PR for that repo as well.

Obviously I'm aware that the security end of this issue isn't super critical given that the library isn't exposed by feather and feather isn't modifying environment variables in many threads (at all), but given it fails the cargo audit check in the CI/CL cycle it should be addressed so that future security advisories are not ignored when cargo audit is used.

Here's the dependency tree from the cargo audit:
image

Related issues

borntyping/rust-simple_logger#41

Checklist

  • Ran cargo fmt, cargo clippy --all-targets, cargo build --release and cargo test and fixed any generated errors!
  • Removed unnecessary commented out code
  • Used specific traces (if you trace actions please specify the cause i.e. the player)

Note: if you locally don't get any errors, but GitHub Actions fails (especially at clippy) you might want to check your rust toolchain version. You can then feel free to fix these warnings/errors in your PR.

… it has addressed the issues present in RUSTSEC-2020-0159 in its own security advisory: RUSTSEC-2020-071.
@Iaiao
Copy link
Contributor

Iaiao commented Nov 7, 2021

You can just remove simple_logger from feather-utils because it's an unused dependency that we somehow forgot to remove from Cargo.toml. Also, I'm not sure we should use simple_logger in minecraft-proxy because we use fern in feather-server, so why not to use the same logging backend in both server and proxy?

simple_asn1 0.6 has already moved to time, but we need to update it to 0.6 by updating it in https://github.com/caelunshun/rsa-der and updating rsa to 0.5.

@noahcoetsee
Copy link
Contributor Author

noahcoetsee commented Nov 7, 2021

Okay. I'll remove the unused deps and switch minecraft_proxy over to fern. I see no reason for it to differ from feather-server's implementation

but we need to update it to 0.6 by updating it in https://github.com/caelunshun/rsa-der and updating rsa to 0.5.

Update: caelunshun has accepted my PR and updated rsa-der to v0.3.0 with the suggested changes.

Note that this still leaves the `rsa` issue until the `pem-rfc7468`/`pkcs8` dependency issues are resolved within that crate.
…account for new `RsaPrivateKey` capitalization.
@Defman
Copy link
Member

Defman commented Nov 8, 2021

Huh, seems like we are depending on an old version of the time crate as well... maybe cargo lock?

@noahcoetsee
Copy link
Contributor Author

Huh, seems like we are depending on an old version of the time crate as well... maybe cargo lock?

Yeah I need to mark this PR as still in progress as I moved on to address the other dependency issues. The old version of time was a dependency of some other dependencies and would generate a similar RUSTSEC segfault error in cargo audit.

@Defman
Copy link
Member

Defman commented Dec 5, 2021

Any updates?

@noahcoetsee
Copy link
Contributor Author

Yeah I've been pretty busy with work and there are some wasm dep. issues that are seemingly irremovable. I'll look into it again in the coming week and see if I can clear it up or not. If not I'll push most of the changes.

@koskja koskja mentioned this pull request Jan 8, 2022
6 tasks
@dlee13
Copy link
Contributor

dlee13 commented Jan 11, 2022

Superseded by #512. This can be closed.

@Defman Defman closed this Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants